Code Review — feat(compass): device-orientation compass widget

Overall this is a clean, well-scoped feature. The API compatibility handling, iOS permission flow, and CSS-custom-property rotation pattern are all solid. A few things worth addressing before merge:

Bugs / Correctness

Missing onRemove on the Leaflet control (src/compass.ts)

The orientation subscription is only cleaned up via map.on('unload', …). If the control is removed programmatically (e.g. control.remove()), the deviceorientation listener leaks. Leaflet controls have a standard lifecycle method for this:

```ts
onRemove() {
if (unsubscribe !== null) unsubscribe();
},
```

The map.on('unload') listener itself also accumulates if the control is added/removed more than once — the unload handler is never de-registered.

lastDeviceHeadingDeg stored in AppState but never read (src/types.ts)

The field is written in compass.ts but nothing in this PR reads it. If it's reserved for a future integration (e.g. the heading-cone wedge using it when GPS course is NaN), a TODO comment on the field would make the intent clear; otherwise it's dead state that adds noise to AppState.

Style / Conventions

Multi-line comment blocks (src/orientation.ts, src/compass.ts)

CLAUDE.md says: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." Both new files have multi-line JSDoc-style blocks above every function. These should be trimmed to a single line each (or removed entirely if the function name makes intent clear).

Minor / Suggestions

deviceorientationabsolute feature-detect (src/orientation.ts:51)

```ts
const eventName = 'ondeviceorientationabsolute' in window ? 'deviceorientationabsolute' : 'deviceorientation';
```

This checks for the event-handler property (which works in Chrome/Android) rather than the event type. A slightly more portable check is 'DeviceOrientationAbsoluteEvent' in window, but the current form works in practice and the fallback to deviceorientation + webkitCompassHeading covers iOS anyway. Not a blocking concern.

No webkitCompassAccuracy guard (src/orientation.ts)

iOS exposes webkitCompassAccuracy (degrees of uncertainty; values ≥ 25 indicate poor calibration). Ignoring it is fine for a v1, but flagging it here in case you want the heading to go stale/null during poor-calibration periods rather than showing a jittery reading.

Click handler attaches before the availability check (src/compass.ts:32–54)

The DeviceOrientationEvent === undefined early-hide runs after the click listener is attached. No real-world impact since the button is hidden, but reversing the order (check availability first, attach listener only if supported) would make the guard more obviously correct.

Test coverage (src/orientation.test.ts)

The five extractHeading tests are thorough and cover the important paths. subscribeOrientation and requestOrientationPermission are side-effectful and untestable without mocks, so skipping them is appropriate.

Summary

The core logic (extractHeading, permission flow, CSS rotation) is correct and well-tested. Fixing the onRemove gap and comment-block style would make this merge-ready.

Cycle-2 fixes pushed:

• onRemove lifecycle hook added; unsubscribe is now a closure variable shared between onAdd and onRemove. Removed the map.on('unload') workaround.
• lastDeviceHeadingDeg dropped from AppState — no readers existed. Visual rotation goes straight to a CSS custom property; reintroduce the field if/when another module needs the numeric value.
• Multi-line comments trimmed to single-line per CLAUDE.md convention in both orientation.ts and compass.ts.

Skipped:

• webkitCompassAccuracy gating — defer until field reports show it's needed.
• 'DeviceOrientationAbsoluteEvent' in window vs 'ondeviceorientationabsolute' — current detection works on Chrome/Android; iOS hits the fallback either way.

Code Review — feat(compass): device-orientation compass widget (#163)

Overview

Clean, focused feature addition. The three-file split (orientation.ts / compass.ts / tests) follows the project's module-per-concern pattern well. Permission gating, heading extraction, and Leaflet control wiring are all nicely separated.

Issues

1. PR description / diff discrepancy — lastDeviceHeadingDeg

The PR summary says:

src/types.ts — lastDeviceHeadingDeg + compassPermission on AppState

But the diff only adds compassPermission. lastDeviceHeadingDeg appears nowhere in the code. This is either a stale description (field was dropped during development) or an omission. If it was intentionally dropped, update the description. If other modules are ever expected to read the current device heading through AppState, it needs to be wired in here.

2. deviceorientationabsolute detection may miss some browsers

// orientation.ts:37
const eventName = 'ondeviceorientationabsolute' in window ? 'deviceorientationabsolute' : 'deviceorientation';

Checking for the on* handler property works on Chrome/Edge but is not guaranteed on every browser — the event can fire without the ondeviceorientationabsolute IDL attribute being present. A more reliable pattern (used by e.g. compass.js) is to try the absolute event first with a one-shot listener, then fall back. That said, for the target platforms (iOS Safari / Android Chrome) the current check behaves correctly today, so this is a low-risk note rather than a blocker.

3. No throttling on orientation events

deviceorientation / deviceorientationabsolute fires up to ~60 Hz on many devices. Each event triggers a setProperty call on the DOM. The 0.12 s CSS transition smooths the visual output well, but the JS callback still runs at full sensor rate. For a single setProperty call this is negligible, but it's worth documenting or throttling if other consumers are added later.

Suggestions

4. The state.compassPermission === 'granted' guard can be tightened

// compass.ts:37
L.DomEvent.on(button, 'click', () => {
  if (state.compassPermission === 'granted') return;
  ...

This is correct, but once granted the button still receives clicks and silently swallows them. Consider either removing the listener after granting or changing the cursor to default so users aren't confused when nothing happens after tapping an already-active compass.

5. unsubscribe closure vs. control lifecycle

The unsubscribe reference lives in addCompassControl's closure but the returned Control instance is never stored:

// compass.ts:62
new Control({ position: 'topright' }).addTo(map);

onRemove (which calls unsubscribe()) is therefore only reachable if the Leaflet map itself tears down the control. For this app's single-page lifetime that's fine, but it's worth a comment in case the control is ever conditionally mounted/unmounted.

Positives

• extractHeading logic is correct and well-justified. The ((x % 360) + 360) % 360 normalization handles negative values and overflow cleanly; the iOS-first preference with graceful W3C fallback matches real-world device behavior.
• iOS permission gate placement is right. Calling requestOrientationPermission() from inside the click handler ensures it's user-gesture–scoped, which iOS 13+ requires.
• Test coverage is solid for the exported pure function. Five edge-case tests cover the most common failure modes (null alpha, NaN, negative webkitCompassHeading, wraparound). The untested browser-API functions (requestOrientationPermission, subscribeOrientation) are reasonable to leave to manual testing given the mocking cost.
• CSS is minimal and correct. Rotating the inner svg rather than the button keeps the tap target stable, and display: none on --unavailable matches the documented "hidden entirely" behaviour.
• void before the floating promise in the click handler is the right idiom here.
• Size budget respected (97.86 kB / 100 kB, +0.75 kB).

Summary

One description inconsistency to resolve (lastDeviceHeadingDeg), one minor UX nit (click guard after grant), and the deviceorientationabsolute detection caveat to keep in mind. Nothing blocking. The core logic is sound.

Code Review — feat(compass): device-orientation compass widget (#163)

Overview

Clean, well-scoped feature. The separation into orientation.ts (sensor logic) and compass.ts (Leaflet control / UI) is a good split. The iOS permission gate, deviceorientationabsolute → deviceorientation fallback, and extractHeading normalisation are all handled correctly. No security or major performance concerns.

Bug — double-tap race condition in compass.ts

src/compass.ts lines 35–48

On non-iOS browsers requestOrientationPermission() resolves synchronously (next microtask). If the user taps the button twice before the first .then() fires, both taps pass the state.compassPermission === 'granted' guard (state hasn't been written yet) and both call subscribeOrientation. The outer unsubscribe variable is overwritten by the second call, leaving the first deviceorientation listener permanently attached.

Fix — add an in-flight guard:

let permissionPending = false;
L.DomEvent.on(button, 'click', () => {
  if (state.compassPermission === 'granted' || permissionPending) return;
  permissionPending = true;
  void requestOrientationPermission().then((permission) => {
    permissionPending = false;
    state.compassPermission = permission;
    // … rest unchanged
  });
});

Minor issues

src/compass.ts — button remains interactive after activation

Once the compass is active the click handler returns early, but the button is still focusable and has no aria-disabled signal. A screen reader will announce "Compass, button" with no indication it can't be activated. Consider button.setAttribute('aria-disabled', 'true') alongside cursor: default when transitioning to the active state.

src/orientation.ts lines 14–17 — double-mod is noisy

return ((360 - event.alpha) % 360 + 360) % 360;

event.alpha is [0, 360) per spec, so 360 - alpha is (0, 360] and the outer + 360) % 360 only matters for the exact alpha === 0 edge case (producing 360). A simpler form with an explicit guard is easier to reason about:

return event.alpha === 0 ? 0 : 360 - event.alpha;

(The webkitCompassHeading normalisation path above it is fine — that one genuinely needs the double-mod.)

What's good

• onRemove unsubscribes the orientation listener — no teardown leak on the happy path.
• CSS custom property (--heading-deg) keeps heading updates off the JS layout path; 0.12s linear is the right call for magnetometer jitter.
• extractHeading test suite covers the iOS preference, W3C flip, out-of-range normalisation, null/NaN fallback, and the mixed NaN case — solid coverage for a pure function.
• Caching compassPermission on AppState correctly avoids re-prompting within a session.
• Bundle impact (+0.75 kB) is trivial; size-limit still passes.

Summary

The double-tap race is worth fixing before merge; the other two points are nice-to-have. Everything else is solid — correct handling of the iOS/W3C heading-direction difference is the trickiest part of this widget and it's done right.